src: make --use-largepages a runtime option #30954

gabrielschulhof · 2019-12-13T23:05:22Z

Moves the option that instructs Node.js to-remap its static code to
large pages from a configure-time option to a runtime option. This
should make it easy to assess the performance impact of such a change
without having to custom-build.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

addaleax

Can you add a basic test that e.g. just spawns node --use-largepages -p 42 and makes sure that the output is correct (ideally verify both stderr and stdout)? That might give us a better idea under which circumstances this is or is not working

doc/api/cli.md

bnoordhuis

LGTM but can you also update doc/node.1 and can I suggest applying this patch so the build won't break on older Linux systems?

diff --git a/src/large_pages/node_large_page.cc b/src/large_pages/node_large_page.cc
index 68fa178b40..d13af4c11e 100644
--- a/src/large_pages/node_large_page.cc
+++ b/src/large_pages/node_large_page.cc
@@ -352,7 +352,7 @@ MoveTextRegionToLargePages(const text_region& r) {
     return -1;
   }
 
-  ret = madvise(tmem, size, MADV_HUGEPAGE);
+  ret = madvise(tmem, size, 14 /* MADV_HUGEPAGE */);
   if (ret == -1) {
     PrintSystemError(errno);
     ret = munmap(tmem, size);

(MADV_HUGEPAGE was added in Linux 2.6.38 and our baseline for running node is 3.x but I don't know what exactly we require for building node.)

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build.

gabrielschulhof · 2019-12-14T22:24:39Z

@addaleax I added the test and I fixed the doc.
@bnoordhuis I replaced the constant with its value and added the man page entry.

src/node.cc

gabrielschulhof · 2019-12-15T07:24:38Z

@bnoordhuis @addaleax @cjihrig @devnexen @gireeshpunathil I changed the implementation to one that accepts three values: 0 (default) to not map, 1 to map but to not report failure, and 2 to map and to report failure on stderr.

src/large_pages/node_large_page.cc

doc/node.1

lundibundi

I think at least IsLargePagesEnabled and MapStaticCodeToLargePages in node_large_page.cc have to be changed as they won't compile/won't be correct on unsupported platforms due to conditional compilation in there. I think just adding conditional #else with correct error ret will be fine. They both won't have a return statement on the unsupported platforms.

src/node.cc

test/parallel/test-startup-large-pages.js

src/node.cc

nodejs-github-bot · 2019-12-17T00:42:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27752/

gabrielschulhof · 2019-12-17T04:53:45Z

@addaleax @joyeecheung I updated the possible values to strings: "off", "silent", "verbose".
@lundibundi I applied your nits, and I added reporting for the case where mapping is not supported. I also placed the whole chunk into an ifdef to prevent compiling the code on unsupported platforms. On those platforms and in the verbose case, a message will be written to stderr that they're not supported.

nodejs-github-bot · 2019-12-17T04:55:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27755/

test/parallel/test-startup-large-pages.js

gabrielschulhof · 2019-12-17T06:47:46Z

@devnexen thanks, updated.

nodejs-github-bot · 2019-12-17T06:49:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27763/

doc/api/cli.md

src/node_options.cc

gabrielschulhof · 2019-12-23T06:25:58Z

Dang! I made a copy/paste error while landing. Sorry I omitted you as a reviewer @jasnell!

bcoe · 2019-12-25T22:51:31Z

@gabrielschulhof we're seeing a bit of a hiccup in our nightly linux tests:

20:01:53 /bin/sh: 1: realpath: not found
20:01:53 gyp: Call to 'realpath src/large_pages/ld.implicit.script' returned exit status 127 while in /home/iojs/build/workspace/node-test-commit-linux-coverage-daily/nodes/benchmark/node.gyp.

Which seems to crop up somewhere between December 21st and December 22nd when this landed. I'm guessing there might be a workaround in changing our compiler flags? Wondering if this is expected however (would guess moving this to a flag was meant to be a noop?).

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: #31063 PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: nodejs#32092 PR-URL: nodejs#30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build. Backport-PR-URL: #32092 PR-URL: #30954 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Co-authored-by: David Carlier <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 13, 2019

addaleax approved these changes Dec 14, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

bnoordhuis approved these changes Dec 14, 2019

View reviewed changes

devnexen approved these changes Dec 14, 2019

View reviewed changes

cjihrig approved these changes Dec 14, 2019

View reviewed changes

src: make --use-largepages a runtime option

a83c5df

Moves the option that instructs Node.js to-remap its static code to large pages from a configure-time option to a runtime option. This should make it easy to assess the performance impact of such a change without having to custom-build.

gabrielschulhof force-pushed the runtime-large-pages-2 branch from c88750e to a83c5df Compare December 14, 2019 22:20

gireeshpunathil reviewed Dec 14, 2019

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

Gabriel Schulhof added 2 commits December 14, 2019 23:28

src: make --use-largepages a three-valued option

65622bf

update test

6e0fd1c

gabrielschulhof force-pushed the runtime-large-pages-2 branch from 6c9b3d5 to 6e0fd1c Compare December 15, 2019 07:28

devnexen reviewed Dec 15, 2019

View reviewed changes

src/large_pages/node_large_page.cc Show resolved Hide resolved

jasnell approved these changes Dec 15, 2019

View reviewed changes

joyeecheung reviewed Dec 16, 2019

View reviewed changes

doc/node.1 Outdated Show resolved Hide resolved

lundibundi reviewed Dec 16, 2019

View reviewed changes

src/node.cc Outdated Show resolved Hide resolved

test/parallel/test-startup-large-pages.js Outdated Show resolved Hide resolved

lundibundi reviewed Dec 16, 2019

View reviewed changes

src/node.cc Show resolved Hide resolved

Gabriel Schulhof added 2 commits December 16, 2019 18:11

heed review comments

a93064d

convert values to strings

764c109

devnexen reviewed Dec 17, 2019

View reviewed changes

test/parallel/test-startup-large-pages.js Outdated Show resolved Hide resolved

Gabriel Schulhof added 2 commits December 16, 2019 22:45

only add sources if arch is x64

67bc994

update test comment

cc54363

lundibundi reviewed Dec 17, 2019

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

src/node_options.cc Show resolved Hide resolved

gabrielschulhof mentioned this pull request Dec 23, 2019

[v12.x] src: make --use-largepages a runtime option #31063

Closed

4 tasks

bcoe mentioned this pull request Dec 26, 2019

build: switch realpath to pwd #31095

Closed

4 tasks

sam-github mentioned this pull request Jan 6, 2020

deps: switch to chromium's zlib implementation #31201

Merged

3 tasks

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

This was referenced Feb 13, 2020

ESM issue on 12.x requires a quick 12.16.1 nodejs/Release#532

Closed

v12.16.1 proposal #31781

Closed

targos mentioned this pull request Feb 18, 2020

v12.16.0 post-mortem nodejs/Release#536

Closed

gabrielschulhof deleted the runtime-large-pages-2 branch February 21, 2020 16:39

targos added the backported-to-v12.x label Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make --use-largepages a runtime option #30954

src: make --use-largepages a runtime option #30954

gabrielschulhof commented Dec 13, 2019

addaleax left a comment

bnoordhuis left a comment

gabrielschulhof commented Dec 14, 2019

gabrielschulhof commented Dec 15, 2019

lundibundi left a comment

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 17, 2019

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 17, 2019

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 23, 2019

bcoe commented Dec 25, 2019

src: make --use-largepages a runtime option #30954

src: make --use-largepages a runtime option #30954

Conversation

gabrielschulhof commented Dec 13, 2019

Checklist

addaleax left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Dec 14, 2019

gabrielschulhof commented Dec 15, 2019

lundibundi left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 17, 2019

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 17, 2019

nodejs-github-bot commented Dec 17, 2019

gabrielschulhof commented Dec 23, 2019

bcoe commented Dec 25, 2019